Skip to content

chore(ci): add fork-side SSO audit script + workflow#32

Merged
awais786 merged 10 commits into
foss-mainfrom
chore/sso-audit-ci
May 16, 2026
Merged

chore(ci): add fork-side SSO audit script + workflow#32
awais786 merged 10 commits into
foss-mainfrom
chore/sso-audit-ci

Conversation

@awais786
Copy link
Copy Markdown

@awais786 awais786 commented May 16, 2026

Summary

Per-fork SSO contract audit in CI. Runs scripts/sso-audit.sh on PRs, blocks merges on security-critical violations. Catches regressions before they reach foss-server-bundle-devstack, whose CI emits ? for fork-side rows.

What it checks

Row Invariant Severity
14 SPA logout MUST NOT call /oauth2/sign_out informational
20 proxy_auth.py MUST import + call django.contrib.auth.logout(request) on identity mismatch security-critical
21 No unanchored ^[^\s@]+@[^\s@]+\.[^\s@]+$ regex in normalisation regression guard

Spec source: sso-rules-moneta/openspec/specs/proxy-auth-middleware/spec.md.

Output

  • Sticky PR comment (header sso-fork-audit, updated on each push)
  • GitHub job summary
  • Exit code 1 on security-critical → merge blocked

Why Row 20 currently fails on this PR

This branch is against foss-main, which doesn't have PR #29 merged. So proxy_auth.py lacks the logout(request) call. The audit is correctly blocking — that's the gate. Two ways to land:

  1. Merge fix: drop stale Django session when proxy identity changes #29 first, then rebase this PR onto foss-main.
  2. Rebase this PR onto fix/proxy-auth-stale-session-on-user-switch temporarily.

Both result in a green audit.

Notes

  • scripts/ was gitignored upstream. .gitignore is changed to scripts/* with a !scripts/sso-audit.sh negation so this one file can be tracked.
  • Pattern templates cleanly to Outline / Penpot / Twenty / SurfSense in follow-up PRs.

🤖 Generated with Claude Code

Adds a per-fork deterministic audit that catches regressions of the
cross-app SSO contract in Pressingly/plane BEFORE they reach
foss-server-bundle-devstack. The bundle's CI runs an audit that emits
`?` for fork-side rows because the forks aren't checked out there;
this PR closes that gap from Plane's side.

What it checks

  scripts/sso-audit.sh runs three deterministic checks against the
  files in this fork that satisfy fork-side rows of
  awais786/sso-rules-moneta:openspec/specs/proxy-auth-middleware/spec.md:

    Row 14 — logout shape: apps/web/core/store/user/index.ts MUST NOT
             call /oauth2/sign_out. Per logout-flow spec the per-app
             button is navigation-only; portal "Logout all" handles
             oauth2-proxy clearing.

    Row 20 — session-identity reconciliation (Rule 2 mismatch flush):
             apps/api/plane/authentication/middleware/proxy_auth.py
             MUST import django.contrib.auth.logout AND invoke
             logout(request). Without it, the stale-session-on-user-
             switch leak returns. SECURITY-CRITICAL — exit 1 on miss.

    Row 21 — polynomial-regex avoidance: proxy_auth.py +
             proxy_auth_utils.py MUST NOT introduce an unanchored
             [^\s@]+@[^\s@]+\.[^\s@]+ regex. Plane uses substring
             check today, so this is a regression guard.

  When upstream sso-rules-moneta adds new fork-side rows, vendor the
  updated check by editing this script.

How it's wired

  .github/workflows/sso-audit.yml runs the script on:
    - pull_request paths: apps/api/plane/authentication/**, apps/web's
      auth-related modules, the script itself, the workflow itself
    - push to foss-main
    - weekly schedule (Mon 09:00 UTC)
    - manual workflow_dispatch

  Output is published in three places:
    - GitHub job summary (always)
    - Sticky PR comment via marocchino/sticky-pull-request-comment@v2
      (PR runs only — one comment per PR, updated on each push)
    - Exit code 1 on security-critical violations → merge blocked

Local dry-run

  On foss-main (pre-PR-29 state): row 20 fires ❌ because
  proxy_auth.py lacks the django_auth.logout import. Exit 1.
  Confirms the gate works.

  On fix/proxy-auth-stale-session-on-user-switch: all three rows ✅.
  Confirms the gate releases when the fix lands.

Note: the audit script lives at scripts/sso-audit.sh which was
otherwise gitignored under `scripts/`. The .gitignore is updated to a
more specific `scripts/*` glob with a `!scripts/sso-audit.sh`
negation so this single file can be tracked while leaving the rest
of scripts/ ignored.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 16, 2026

Plane SSO Fork Audit

Cross-app contract: https://github.com/awais786/sso-rules-moneta/blob/main/openspec/specs/proxy-auth-middleware/spec.md
Row numbers match the 21-row table at https://github.com/awais786/sso-rules-moneta/blob/main/skills/app-rules/SKILL.md#5-report

Row Invariant Status Notes
14 logout shape: SPA logout does not call /oauth2/sign_out apps/web/core/store/user/index.ts does not invoke /oauth2/sign_out (this row verifies only that the SPA doesn't try to clear the upstream proxy cookie itself; that's the portal's job. The SPA MAY still POST /auth/sign-out/ — tolerated drift, see logout-flow spec)
20 session-identity reconciliation present (Rule 2 mismatch flush) django.contrib.auth.logout imported (as logout) and invoked at least once in apps/api/plane/authentication/middleware/proxy_auth.py — Rule 2 mismatch flush in place
21 email-shape detection uses substring/indexOf, not polynomial regex No polynomial-backtracking email-shape regex in apps/api/plane/authentication/middleware/proxy_auth.py apps/api/plane/authentication/middleware/proxy_auth_utils.py; using substring check or other O(n) detection

All fork-side invariants hold.

@awais786 awais786 requested a review from Copilot May 16, 2026 09:33
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a deterministic fork-side SSO contract audit to this Plane fork, wiring it into GitHub Actions so regressions in key SSO invariants (especially the stale-session/user-switch class) are caught before they reach the bundle CI.

Changes:

  • Added scripts/sso-audit.sh to audit a subset of cross-app SSO contract rows via grep-based checks and emit a markdown table.
  • Added .github/workflows/sso-audit.yml to run the audit on PR/push/schedule, publish to job summary, and post a sticky PR comment.
  • Updated .gitignore to allow tracking scripts/sso-audit.sh while keeping the rest of scripts/ ignored.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.

File Description
scripts/sso-audit.sh Implements the fork-side SSO audit checks and emits a markdown report + exit code gating.
.gitignore Un-ignores scripts/sso-audit.sh while keeping other scripts/ contents ignored.
.github/workflows/sso-audit.yml Runs the audit in CI, publishes the report, and gates merges on security-critical failures.

Comment thread scripts/sso-audit.sh Outdated
Comment on lines +25 to +29
# Rows covered:
# Row 14 — logout shape: SPA logout MUST NOT call /oauth2/sign_out, MUST
# redirect to portal host (env-supplied or 4-label regex), MUST
# NOT POST /auth/sign-out/ (per logout-flow spec §"Per-app
# 'Logout' SHALL be navigation-only"; tolerated as drift today).
Comment thread scripts/sso-audit.sh Outdated
Comment on lines +124 to +131
# Detect either `from django.contrib.auth import logout` (any form) or
# an aliased import that brings `logout` into scope, plus a call site.
local has_import
has_import=$(grep -cE '^from django\.contrib\.auth import (.*\b)?logout(\b.*)?$|^from django\.contrib\.auth import logout as ' "$PROXY_AUTH" || true)

local has_call
has_call=$(grep -cE '\blogout\(request\)|\bdjango_logout\(request\)' "$PROXY_AUTH" || true)

Comment thread scripts/sso-audit.sh Outdated
echo "## Plane SSO Fork Audit"
echo
echo "Cross-app contract: \`awais786/sso-rules-moneta:openspec/specs/proxy-auth-middleware/spec.md\`"
echo "Row numbers match \`skills/app-rules/SKILL.md\` §5 (the 21-row table)."
Comment thread .github/workflows/sso-audit.yml Outdated
Comment on lines +57 to +62
- name: Post sticky PR comment
if: github.event_name == 'pull_request' && always()
uses: marocchino/sticky-pull-request-comment@v2
with:
header: sso-fork-audit
path: audit-output.md
All four comments from the PR #32 review are valid. Fixes:

#1 (sso-audit.sh:29) — Row 14 description claimed three checks
   (no /oauth2/sign_out, portal-host redirect, no POST /auth/sign-out/)
   but the function only verifies the first. Narrows the comment to
   match what's actually implemented. The other two properties from
   logout-flow spec need runtime context or AST analysis; not in scope
   for this deterministic gate.

#2 (sso-auth.sh:131) — Row 20 detection was brittle:
   - Single-line regex missed parenthesized multiline imports
     (`from django.contrib.auth import (\n  login,\n  logout,\n)`)
   - Call regex required exact `logout(request)` — missed whitespace
     (`logout( request )`), keyword form (`logout(request=request)`),
     and aliased call sites (`auth_logout(request)`)
   Could false-fail on legitimate fixes → block merges spuriously.

   New detection uses Python's `ast` module to parse the proxy_auth.py
   import block and extract the in-scope name for
   django.contrib.auth.logout (alias or plain). Then a whitespace-tolerant
   grep checks for a call to that name. Handles every variant in one
   pass without writing multi-line regex in bash. Verified against a
   synthetic fixture with aliased + parenthesized + whitespaced call.

#3 (sso-audit.sh:191) — Output referenced `skills/app-rules/SKILL.md`,
   which lives in awais786/sso-rules-moneta, not in Pressingly/plane.
   Confused readers of the sticky PR comment. Replaced both repo-
   relative references with full GitHub URLs so the links work from
   the PR view.

#4 (.github/workflows/sso-audit.yml:62) — `marocchino/sticky-pull-
   request-comment@v2` needs `pull-requests: write`. PRs from external
   forks get a read-only GITHUB_TOKEN regardless of declared
   permissions, and the action errors out, marking the whole workflow
   failed even when the audit passed.

   Two-layer fix:
     - Skip the comment step on fork PRs via
       `github.event.pull_request.head.repo.full_name == github.repository`
     - Add `continue-on-error: true` as a defensive belt so other
       comment-posting failures (rate limit, GitHub API blip) don't
       fail the audit gate either

   The audit gate itself (final exit-1 step) is unaffected — still
   blocks merge on security-critical violations regardless of comment
   posting.

Local dry-run on foss-main still produces the expected red ❌ on row
20 with the new alias-aware detection. Switching to the fix branch
detects the import (as `logout`) and the call site correctly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

Comment thread scripts/sso-audit.sh
Comment on lines +165 to +169
# Look for a call to <logout_name>(...) anywhere in the file. The argument
# can include whitespace, line breaks, or `request=request` keyword form;
# the audit only cares that the call exists, not its exact shape.
local call_pattern="\\b${logout_name}[[:space:]]*\\("
if grep -qE "$call_pattern" "$PROXY_AUTH"; then
awais786 and others added 2 commits May 16, 2026 14:54
Copilot review on PR #32 flagged that the `\b` word-boundary in the
row-20 grep is a GNU `grep -E` extension and undefined on BSD / POSIX-
strict implementations. macOS / Alpine / busybox grep may interpret it
as literal backspace or literal `b`, causing the audit to silently
miss real `logout(...)` calls and false-fail rows that should pass.

Replaced with POSIX character-class boundary:

  (^|[^[:alnum:]_])<alias>[[:space:]]*\(

`(^|[^[:alnum:]_])` matches start-of-line OR any non-word character
before the alias name — semantically identical to `\b` at the left
edge, but using only ERE constructs that work in every grep
implementation. The right edge doesn't need a boundary because
`[[:space:]]*\(` already requires the next non-space character to be
`(`, which disambiguates `logout(` from `logout_more(`.

Verified against six fixtures:
  - `logout(request)`                              → match ✅
  - `auth_logout( request )`                       → match ✅
  - `    logout(request=request)`                  → match ✅
  - `my_xauth_logout(request)` vs alias=auth_logout → skip ✅
  - leading start-of-line                          → match ✅
  - `something_logout(request)` vs alias=logout    → skip ✅

The fifth Copilot comment is the last open one; the previous four are
addressed by commit 4fb4805.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ified

The previous message "navigation-only logout shape preserved" overstated
what the check actually verifies. The audit only confirms the SPA does
NOT call /oauth2/sign_out. It does NOT verify:
  - whether the SPA POSTs /auth/sign-out/ (it does today — tolerated drift)
  - what host the SPA navigates to after logout
  - whether portal "Logout all" is wired correctly

The new message states exactly what's checked and acknowledges the
tolerated POST /auth/sign-out/ drift so readers don't misread the ✅ as
"per-app Logout is fully spec-conformant."

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

scripts/sso-audit.sh:212

  • Row 21 uses raw grep -n hits (including full line contents) in the Notes column. As with row 14, any | characters or newlines in the matched lines will corrupt the Markdown table output. Safer options: emit only file:line locations, or escape | and replace newlines with <br> before printing.
  local hits
  hits=$(grep -nE '\[\^[\\\\]s@\]\+@\[\^[\\\\]s@\]\+\\\.\[\^[\\\\]s@\]\+' "${files[@]}" 2>/dev/null || true)

  if [[ -n "$hits" ]]; then
    record 2 "❌" "Polynomial-backtracking email-shape regex detected: $hits. Rewrite to indexOf/substring-based check per openspec proxy-auth-middleware §'email-shape detection SHALL avoid polynomial-backtracking regex'. Reference: Outline's normalizeProxyEmail in Pressingly/outline#19."
    return

Comment thread scripts/sso-audit.sh
Comment on lines +99 to +101
local line
line=$(grep -nE '/oauth2/sign_?out' "$LOGOUT_SPA" | head -1)
record 0 "❌" "Logout calls \`/oauth2/sign_out\` at $LOGOUT_SPA:$line — per logout-flow spec, per-app Logout is navigation-only. Drop the call; portal 'Logout all' handles oauth2-proxy clearing."
Comment on lines +39 to +52
steps:
- name: Checkout
uses: actions/checkout@v4

- name: Run fork audit
id: audit
run: |
set -o pipefail
if bash scripts/sso-audit.sh | tee audit-output.md; then
echo "audit_exit=0" >> "$GITHUB_OUTPUT"
else
echo "audit_exit=$?" >> "$GITHUB_OUTPUT"
fi

Comment thread scripts/sso-audit.sh Outdated
Comment on lines +32 to +35
permissions:
contents: read
pull-requests: write

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 8a1b118. I split the workflow into two jobs for least privilege: the audit job now runs with contents: read only, and a separate PR-only same-repo comment job has pull-requests: write and posts the sticky comment from an uploaded/downloaded audit artifact. Security-critical audit gating behavior is preserved.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@awais786
Copy link
Copy Markdown
Author

Status check on open Copilot review threads (8 of 9 unreplied) after recent commits:

Thread Comment Status
#3252607021 Row 14 description over-claims Addressed by ce67aa6025 — Row 14 description now explicitly says it does NOT verify the portal-host redirect or no POST /auth/sign-out/ properties.
#3252607034 Row 20 brittle for aliased imports Addressed by 4fb4805b55 + ece9f69841 — Row 20 now uses Python ast to resolve the import alias (handles from django.contrib.auth import logout as django_logout, multiline parens, mixed imports), then POSIX-boundary grep for the call.
#3252607044 skills/app-rules/SKILL.md path doesn't exist False positive — verified the file exists at https://github.com/awais786/sso-rules-moneta/blob/main/skills/app-rules/SKILL.md. The audit output's URL is correct.
#3252607062 Sticky-comment needs PR write on fork PRs Addressed by 19d580a830 + 8a1b1180a3 — workflow now splits into two jobs; the comment job is gated on github.event.pull_request.head.repo.full_name == github.repository so it no-ops on fork PRs (where the token has no PR write anyway).
#3252634684 \b GNU-only in grep -E Addressed by ece9f69841call_pattern now uses (^|[^[:alnum:]_])${logout_name}[[:space:]]*\\( (POSIX).
#3252653011 Row 14 source line embedded in markdown table breaks rendering Unresolved — the audit's Row 14 failure note interpolates the matched line including any | characters into the table cell. @copilot apply changes based on this feedback.
#3252653019 Workflow doesn't set up python3 Unresolved — Row 20's Python ast parser depends on python3, but the workflow has no actions/setup-python step. Ubuntu runners ship Python 3 by default so this is currently working by accident. @copilot apply changes based on this feedback — add an explicit actions/setup-python@v5 step before the audit.
#3252653022 Exit code header comment misstates behaviour Addressed in current header — exit code 0 now correctly described as "no SECURITY-CRITICAL rows failed; non-security rows may still report ❌".
#3252653024 Workflow-level pull-requests: write Already replied: Implemented in 8a1b1180a3 (two-job split).

Net unresolved: 2 (markdown source-line break + Python setup). Both have @copilot apply changes triggers above.

Copy link
Copy Markdown

Copilot AI commented May 16, 2026

Status check on open Copilot review threads (8 of 9 unreplied) after recent commits:

| Thread | Comment | Status |...

Implemented in 8ae1883 and ea7d9ef. I addressed both unresolved items: (1) markdown-safe table notes in scripts/sso-audit.sh (pipes/newlines are escaped for stable rendering), and (2) explicit Python provisioning in CI via actions/setup-python@v5 (python-version: 3.12) before running the audit.

@awais786 awais786 merged commit 7be0f68 into foss-main May 16, 2026
10 checks passed
@awais786 awais786 deleted the chore/sso-audit-ci branch May 16, 2026 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants